Fix the interpreted version of Expression<>.Compile() for setting field on value types #116901

manofstick · 2025-06-22T07:22:20Z

A bug was discovered where Expression<>.Compile(true) would return a different result to Expression<>.Compile(false). The result in the case of the interpreter meant that fields of a value type embedded within a reference type could not be set, as the interpreter was setting the fields on a boxed version of the value type.

The following unit test demonstrates this functionality:

        class Class<T>
        {
            public T Item;
        }

        struct Struct<T>
        {
            public T Item;
        }

        [Theory]
        [ClassData(typeof(CompilationTypes))]
        public static void AssignToNestedValueTypesClassStructIntTest(bool useInterpreter)
        {
            var outer =
                Expression.Parameter(typeof(Class<Struct<int>>));

            Expression<Action<Class<Struct<int>>>> e =
                Expression.Lambda<Action<Class<Struct<int>>>>(
                    Expression.Assign(
                        Expression.Field(
                            Expression.Field(
                                outer,
                                "Item"
                            ),
                            "Item"
                        ),
                        Expression.Constant(42)
                    ),
                    [outer]
                );

            Action<Class<Struct<int>>> f = e.Compile(useInterpreter);

            Class<Struct<int>> src = new Class<Struct<int>>();

            Assert.Equal(0, src.Item.Item);

            f(src);

            Assert.Equal(42, src.Item.Item); // fails for interpreted version
        }

This PR fixes this issue by utilizing TypedReference to access the fields. A special type FieldData was added into the InterpretedFrame's Data stack which is then used to determine if the slot represents a value type field access. Externally what was exposed as an object array is now wrapped, and access unwraps to the actual object upon access.

The FieldOperations class is the only class that is required to handle the added FieldData type.

NOTE: Tests for the System.Linq.Expressions library had been ignored in debug builds (due to speed reasons) but I have commented out that in order that they are executed. So presumably that change would be reversed before this PR is pulled into the mainline...

manofstick · 2025-06-25T06:06:12Z

@333fred @cston you are both listed as owners of System.Linq.Expressions so just trying to make sure this hits your radar.

Obviously due to this there isn't too much life in this area, but this is a bug fix to an issue that I have had at work.

But this PR, alongside this issue - which I note was apparently fixed, but not merged - are proving that the interpreted "compile" operation is somewhat flakey in it's current incarnation.

So to be it should either be marked as obsolete, or there bugs fixes should make it into the mainline.

Anyways, the errors that I'm seeing that are generated in the build here are not caused by this PR - I have run the test suite on both Windows and Linux (Ubuntu) and they all pass - so I'm not sure if I can do anything more other than continue to merge in mainline changes...

jkotas · 2025-06-25T12:51:48Z

Anyways, the errors that I'm seeing that are generated in the build here are not caused by this PR

Mono and NativeAOT legs have failing System.Linq.Expressions.Tests tests related to this PR:

[07:39:35] info: [FAIL] System.Linq.Expressions.Tests.Compiler_Tests.NewStructWithArgsAndMemberInitializer(useInterpreter: True)
[07:39:35] info: Assert.Equal() Failure: Values differ
[07:39:35] info: Expected: 6
[07:39:35] info: Actual:   5
[07:39:35] info:    at System.Linq.Expressions.Tests.Compiler_Tests.NewStructWithArgsAndMemberInitializer(Boolean useInterpreter)
[07:39:35] info:    at System.Object.InvokeStub_Compiler_Tests.NewStructWithArgsAndMemberInitializer(Object , Span`1 )
[07:39:35] info:    at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
[07:39:35] info: [FAIL] System.Linq.Expressions.Tests.Compiler_Tests.NewStructWithStructMemberMemberInitializer(useInterpreter: True)
[07:39:35] info: Assert.Equal() Failure: Values differ
[07:39:35] info: Expected: 7
[07:39:35] info: Actual:   0
[07:39:35] info:    at System.Linq.Expressions.Tests.Compiler_Tests.NewStructWithStructMemberMemberInitializer(Boolean useInterpreter)
[07:39:35] info:    at System.Object.InvokeStub_Compiler_Tests.NewStructWithStructMemberMemberInitializer(Object , Span`1 )
[07:39:35] info:    at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
[07:39:35] info: [FAIL] System.Linq.Expressions.Tests.Compiler_Tests.NewClassWithMemberListOfStructInitializer(useInterpreter: True)
[07:39:35] info: Assert.Equal() Failure: Values differ
[07:39:35] info: Expected: 7
[07:39:35] info: Actual:   0
[07:39:35] info:    at System.Linq.Expressions.Tests.Compiler_Tests.NewClassWithMemberListOfStructInitializer(Boolean useInterpreter)
[07:39:35] info:    at System.Object.InvokeStub_Compiler_Tests.NewClassWithMemberListOfStructInitializer(Object , Span`1 )
[07:39:35] info:    at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
[07:39:38] info: Finished:    System.Linq.Expressions.Tests.dll

jkotas · 2025-06-25T12:59:29Z

the interpreted "compile" operation is somewhat flakey in it's current incarnation.

Yes, the System.Linq.Expression interpreter has limitations that cause it to produce different results. The one you are trying to fix has to do with not being able to represent byrefs with full fidelity. #19286 is an example of observable behavior difference caused by the same limitation.

manofstick · 2025-06-26T06:55:32Z

Yes, the System.Linq.Expression interpreter has limitations that causes it to produce different results. The one you are trying to fix has to do with not being able to represent byrefs with full fidelity. #19286 is an example of observable behavior difference caused by the same limitation.

Hi @jkotas, thanks for taking the time to have a bit of a look.

That example is a bit different, although the above issue possibly could be resolved in the similar way (although I'm not sure). This is not for shared references, this is purely to denote the case for when you have a value type field inside another object and you need to set it's value.

struct Inner { public int Value; }
class Outer { public Inner Inner;  }

...

outer.Inner.Value = 42; // this currently fails to set the correct version due to the boxed version added

So the PR added a delayed object to the stack which is the root along with field accessors. This can (and is in most cases) just realized back to the actual object upon access (where the array access is now indirect through a wrapper) but also handles the case where the value is being set through the TypedReference mechanism.

Anyway, I could have a look at the #19286 if there is the possibility that these will end up being merged?

Have a good day!

jkotas · 2025-06-26T07:10:32Z

Anyway, I could have a look at the #19286 ?

#19286 cannot be fixed in System.Linq.Expression interpreter alone. System.Linq.Expression interpreter is built on top of reflection. The problem is that reflection does not have full support for byrefs. We would have to add new reflection APIs that provide full support for byrefs first. There is more discussion about it in the issue.

In any case, high fidelity byref support in the interpreter sounds like a significant new feature to me. I do not expect it to be merged per https://github.com/dotnet/runtime/blob/main/src/libraries/System.Linq.Expressions/README.md .

manofstick · 2025-06-26T09:03:44Z

Mono and NativeAOT legs have failing System.Linq.Expressions.Tests tests related to this PR:

OK I believe that these have been resolved now - actually just removed what I'd added to make the code look "nicer" (more symmetrical) but that had lead to the use of __makeref which obviously mono/aot must have an issue with I guess?

manofstick added 3 commits June 22, 2025 15:35

Add tests showing buggy behaviour

8866da3

Add FieldData object to represent valuetype field operation; wrap access

7a8f4a9

Utilize GetValueDirect

669ca7d

github-actions bot added the area-System.Linq.Expressions label Jun 22, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 22, 2025

Merge branch 'dotnet:main' into fix-linq-set-field-on-value-types

e7bb6d2

Remove use of __makeref to hopefully fix aot issue

e60d5c9

build-analysis bot mentioned this pull request Jun 26, 2025

wasm build failure in CI #117017

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix the interpreted version of Expression<>.Compile() for setting field on value types #116901

Fix the interpreted version of Expression<>.Compile() for setting field on value types #116901

manofstick commented Jun 22, 2025 •

edited

Loading

Uh oh!

manofstick commented Jun 25, 2025

Uh oh!

jkotas commented Jun 25, 2025

Uh oh!

jkotas commented Jun 25, 2025 •

edited

Loading

Uh oh!

manofstick commented Jun 26, 2025

Uh oh!

jkotas commented Jun 26, 2025 •

edited

Loading

Uh oh!

manofstick commented Jun 26, 2025

Uh oh!

Uh oh!

Fix the interpreted version of Expression<>.Compile() for setting field on value types #116901

Are you sure you want to change the base?

Fix the interpreted version of Expression<>.Compile() for setting field on value types #116901

Conversation

manofstick commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manofstick commented Jun 25, 2025

Uh oh!

jkotas commented Jun 25, 2025

Uh oh!

jkotas commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manofstick commented Jun 26, 2025

Uh oh!

jkotas commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manofstick commented Jun 26, 2025

Uh oh!

Uh oh!

manofstick commented Jun 22, 2025 •

edited

Loading

jkotas commented Jun 25, 2025 •

edited

Loading

jkotas commented Jun 26, 2025 •

edited

Loading